Skip to content

fix(plugins): replace dependency marker files with a real satisfaction check#390

Merged
ChuckBuilds merged 5 commits into
mainfrom
fix/plugin-dependency-satisfaction-check
Jul 11, 2026
Merged

fix(plugins): replace dependency marker files with a real satisfaction check#390
ChuckBuilds merged 5 commits into
mainfrom
fix/plugin-dependency-satisfaction-check

Conversation

@ChuckBuilds

@ChuckBuilds ChuckBuilds commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

  • The .dependencies_installed hash-marker system only tracked "was this exact requirements.txt hashed before," not whether the packages it names are actually installed — fragile against wiped venvs, manually removed packages, or lost/corrupted markers, and dead weight for the ~10 plugins whose requirements.txt is comment-only.
  • Adds requirements_are_satisfied() in plugin_loader.py, which checks each real requirement line against importlib.metadata directly, so install_dependencies() only shells out to pip when something is actually missing or version-mismatched.
  • Removes the marker file entirely: marker read/write sites in plugin_loader.py/store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation in plugin_manager.py, and the already-stale scripts/clear_dependency_markers.sh.
  • Adds packaging as an explicit top-level dependency (was only a transitive dep of pytest before).

Test plan

  • ./venv/bin/python3 -m pytest test/test_plugin_loader.py test/test_store_manager_caches.py -q — 53 passed
  • ./venv/bin/python3 -m pytest test/test_plugin_system.py -q --deselect test/test_plugin_system.py::TestPluginHealth::test_circuit_breaker — 10 passed, 1 deselected (pre-existing unrelated failure — stale mock signature in a cache_manager test, confirmed failing in isolation on this branch's base too)
  • New test asserting an already-satisfied real dependency (pytest) skips the pip subprocess entirely
  • Manually verified requirements_are_satisfied() against real plugin requirements.txt files (vacuous and real-deps cases) and edge cases (unmet packages, impossible version specs, -r pip-option lines)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ

Summary by CodeRabbit

  • New Features

    • Plugin dependency checks now recognize already-satisfied requirements and skip unnecessary installations.
    • Dependency requirements are validated more accurately, including support for version constraints.
  • Bug Fixes

    • Removed reliance on dependency marker files, improving consistency when plugin requirements change.
    • Simplified plugin update handling for untracked files.
    • Added coverage for skipping installation when dependencies are already available.

…n check

The .dependencies_installed hash-marker system only tracked "was this exact
requirements.txt hashed before" — not whether the packages it names are
actually present. That made it fragile (a wiped venv, a manually removed
package, or a lost/corrupted marker forces a needless full pip reinstall or,
worse, a false skip) and produced dead weight for the ~10 plugins whose
requirements.txt is comment-only (they still paid a pip subprocess on first
boot before a marker existed).

Replace it with requirements_are_satisfied() in plugin_loader.py, which
checks each real requirement line against importlib.metadata directly, so
install_dependencies() only shells out to pip when something is actually
missing or version-mismatched. Drops the marker file entirely: removed all
marker read/write sites in plugin_loader.py and store_manager.py, the
now-pointless marker-cleanup step in the git-update path, the unused legacy
marker implementation in plugin_manager.py, and the already-stale
clear_dependency_markers.sh script.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@ChuckBuilds, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 87c8dc3d-1afb-42d9-ac4b-82fb22ed01e7

📥 Commits

Reviewing files that changed from the base of the PR and between d86dc59 and 22f0a96.

📒 Files selected for processing (5)
  • src/common/permission_utils.py
  • src/plugin_system/plugin_loader.py
  • src/plugin_system/store_manager.py
  • test/test_permission_utils.py
  • test/test_plugin_loader.py
📝 Walkthrough

Walkthrough

Plugin dependency installation now parses requirements and checks installed versions before invoking pip. Marker-file tracking and duplicate installers were removed, store updates adjust stashing behavior, and tests cover satisfied dependencies and retry paths.

Changes

Plugin dependency installation

Layer / File(s) Summary
Requirement parsing and loader short-circuits
requirements.txt, src/plugin_system/plugin_loader.py
Adds the packaging dependency, parses real requirements, skips satisfied or empty installations, and removes dependency marker writes.
Manager and store dependency flow
src/plugin_system/plugin_manager.py, src/plugin_system/store_manager.py
Removes the manager’s duplicate installer, applies preflight checks in store installation, and stashes all untracked files during updates.
Dependency behavior tests
test/test_plugin_loader.py, test/test_plugin_system.py
Covers satisfied requirements, retry behavior, and dependency-check test ownership.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: replacing dependency marker files with a real dependency satisfaction check.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/plugin-dependency-satisfaction-check

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/plugin_system/plugin_loader.py Fixed
Comment thread src/plugin_system/plugin_loader.py Fixed
@codacy-production

codacy-production Bot commented Jul 10, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -6 complexity · -2 duplication

Metric Results
Complexity -6
Duplication -2

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/plugin_system/store_manager.py (1)

1900-1923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated preflight-check pattern with plugin_loader.install_dependencies.

The requirements_has_real_deps / requirements_are_satisfied guard-and-log sequence here is nearly identical to the one in src/plugin_system/plugin_loader.py's install_dependencies (lines 261-273). Consider extracting a small shared helper (e.g. should_skip_pip_install(requirements_file, label, logger)) that both call sites can reuse, since the two pip-invocation strategies (subprocess vs install_requirements_file) can remain separate.

♻️ Possible shared helper
def should_skip_pip_install(requirements_file: str, label: str, logger) -> bool:
    if not requirements_has_real_deps(requirements_file):
        logger.debug("requirements.txt for %s has no real dependencies, skipping pip", label)
        return True
    if requirements_are_satisfied(requirements_file):
        logger.debug("Dependencies for %s already satisfied, skipping pip", label)
        return True
    return False
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/plugin_system/store_manager.py` around lines 1900 - 1923, Extract the
duplicated requirements preflight checks from _install_dependencies and
plugin_loader.install_dependencies into a shared should_skip_pip_install helper,
reusing requirements_has_real_deps and requirements_are_satisfied with
consistent debug logging. Update both callers to invoke the helper while
preserving their existing, separate pip installation strategies and the
missing-file handling in _install_dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 26-93: Address CodeQL’s uncontrolled-path warnings in
requirements_has_real_deps and requirements_are_satisfied by making each
function explicitly validate or sanitize requirements_file before calling
open(). Reuse the module’s established realpath/basename containment approach,
or change the functions to accept a prevalidated Path type and enforce the
allowed directory boundary internally; update their callers accordingly while
preserving existing error behavior.

---

Nitpick comments:
In `@src/plugin_system/store_manager.py`:
- Around line 1900-1923: Extract the duplicated requirements preflight checks
from _install_dependencies and plugin_loader.install_dependencies into a shared
should_skip_pip_install helper, reusing requirements_has_real_deps and
requirements_are_satisfied with consistent debug logging. Update both callers to
invoke the helper while preserving their existing, separate pip installation
strategies and the missing-file handling in _install_dependencies.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ada0a19e-e8d2-4b39-9438-6e21851f72ac

📥 Commits

Reviewing files that changed from the base of the PR and between 978a03b and d86dc59.

📒 Files selected for processing (7)
  • requirements.txt
  • scripts/clear_dependency_markers.sh
  • src/plugin_system/plugin_loader.py
  • src/plugin_system/plugin_manager.py
  • src/plugin_system/store_manager.py
  • test/test_plugin_loader.py
  • test/test_plugin_system.py
💤 Files with no reviewable changes (2)
  • src/plugin_system/plugin_manager.py
  • scripts/clear_dependency_markers.sh

Comment thread src/plugin_system/plugin_loader.py
…ecks

CodeQL flagged 2 new high-severity "uncontrolled data used in path
expression" alerts at the open() calls inside this PR's new
requirements_has_real_deps()/requirements_are_satisfied() -- both are
reachable from paths that were never run through the basename+trusted-base
sanitiser this codebase already uses elsewhere:

- PluginLoader.install_dependencies() only applied that sanitiser when its
  optional plugins_dir argument was actually passed; the "no plugins_dir"
  branch trusted plugin_dir_real directly. Made plugins_dir required (not
  Optional) so that branch can't exist, and added an explicit guard in
  load_plugin() so install_deps=True without a plugins_dir fails loudly
  instead of silently. Production's only real caller (PluginManager) always
  passes plugins_dir already; the harness/dev-server/render-plugin callers
  all use install_deps=False and are unaffected.

- StoreManager._install_dependencies() never sanitised plugin_path at all,
  and its call sites ultimately derive that path from a plugin's own
  manifest.json "id" field (install_plugin_from_url) -- a malicious plugin
  could otherwise point requirements_file outside plugins_dir. Applied the
  same os.path.basename()-based containment pattern PluginLoader already
  uses (and that CodeQL recognises as a real sanitiser).

Added test_install_dependencies_requires_plugins_dir and
test_install_dependencies_rejects_path_outside_plugins_dir to lock in the
actual security property, not just quiet the scanner. Verified: all 20
tests in test_plugin_loader.py pass, plus the PR's existing test plan
(test_plugin_system.py, test_store_manager_caches.py: 53 passed) and the
full CI plugin-safety suite (test_harness.py, test_visual_rendering.py,
test_plugin_matrix.py: 52 passed, 2 pre-existing skips) all still pass.
Comment thread src/plugin_system/plugin_loader.py Fixed
…tion check

The previous commit's os.path.basename() + os.path.join() pattern (which a
pre-existing code comment claimed CodeQL recognises as a sanitiser) did not
actually clear the alert -- the next CodeQL run still flagged the same 2
sink lines, plus a new one at the os.path.join() call itself. Taking a
substring of tainted data apparently isn't treated as a barrier by this
query, whatever the comment assumed.

Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir
via os.scandir() and only use a name that scandir itself produced, matched
by equality against the caller's requested name. The path is then built
from that enumerated entry, not from the caller's string -- a value
sourced from iterating a trusted, non-tainted directory carries no taint
regardless of what it happens to equal, which is a stronger and more
conventional allowlist-style barrier than string-stripping. Applied
identically in both PluginLoader.install_dependencies() and
StoreManager._install_dependencies(), sharing one implementation.

Re-verified: all 65 tests across test_plugin_loader.py (20, including the
2 new security regression tests), test_store_manager_caches.py (35),
test_plugin_system.py (10) pass, plus the full CI plugin-safety suite
(test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52
passed, 2 pre-existing skips).
…re logging

CodeQL flagged 3 clear-text-logging-of-secrets alerts in
install_requirements_file() (src/common/permission_utils.py:353,360,371).
Pre-existing on main, unrelated to this PR's own diff, but now visible
since the path-injection alerts that previously took priority in the
annotation list are fixed.

The underlying risk is real: pip can echo a private index URL's embedded
basic-auth credentials (from a requirements.txt --index-url line or
PIP_INDEX_URL) back verbatim in its own stderr/stdout on failure, and this
function both logs that output directly and returns it to callers --
store_manager.py's _install_dependencies() logs result.stderr from this
same function too.

Added _redact_url_credentials(), applied immediately after each of the two
subprocess.run() calls (mutating result.stderr/stdout in place) rather
than patching each log call site individually. This closes the leak at
the source: every downstream use -- the three flagged log lines, the
"note" string embedded in the returned stdout, and store_manager.py's own
logging of the returned result -- gets the redacted text for free.

Verified the fixed-phrase "denied" check (`"a password is required" in
result.stderr`) is unaffected, since URL syntax and those phrases don't
overlap -- covered explicitly by
test_does_not_touch_denied_check_phrases. Added
test/test_permission_utils.py (6 tests) covering the redaction helper
directly and both subprocess.run() call sites (the sudo-wrapper branch,
which this repo's scripts/fix_perms/safe_pip_install.sh makes live, and
the no-wrapper fallback branch). All pass.
The previous commit's redaction (mutating result.stderr/stdout right after
each subprocess.run()) didn't clear CodeQL's clear-text-logging alerts --
same lesson as the path-injection fix earlier in this PR: a static
analyzer can't tell "this value was already sanitised two lines up" from
"this is still the raw tainted value" just by looking at a single log
call in isolation, so it conservatively keeps flagging it regardless of
what the redaction function actually does.

Removed all dynamic interpolation (req_file, result.stderr) from the 3
flagged logger.warning() calls entirely, replacing them with fixed
messages plus (for the one that had it) result.returncode, which is a
plain int with no possible taint. The full redacted detail is still
available where it actually matters -- in the returned
CompletedProcess.stderr/stdout and the "note" text -- just not duplicated
into a log line a scanner has to reason about in isolation.

Re-verified: all 6 test_permission_utils.py tests still pass (they assert
on the returned result, not log call arguments), plus the full
test_plugin_loader.py/test_store_manager_caches.py/test_plugin_system.py
suite (71 passed, 1 pre-existing deselect, 4 subtests).
@ChuckBuilds ChuckBuilds merged commit 05e7c43 into main Jul 11, 2026
7 checks passed
@ChuckBuilds ChuckBuilds deleted the fix/plugin-dependency-satisfaction-check branch July 11, 2026 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants